Skip to content

feat: merklelization of lists #528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 53 commits into from
Jan 12, 2024

Conversation

Godspower-Eze
Copy link
Contributor

Closes #482

@Godspower-Eze Godspower-Eze requested a review from a team as a code owner December 14, 2023 23:43
@Godspower-Eze Godspower-Eze marked this pull request as draft December 14, 2023 23:43
@Godspower-Eze Godspower-Eze marked this pull request as ready for review January 9, 2024 22:47
Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments (only minor ones about performance and style). Really good work!

interior_count = node_count - leaf_count
leaf_start = interior_count * @bytes_per_chunk
padded_chunks = chunks |> convert_to_next_pow_of_two(leaf_count)
buffer = <<0::size(leaf_start * @bits_per_byte), padded_chunks::bitstring>>
Copy link
Collaborator

@MegaRedHand MegaRedHand Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave this for the next PR, but using a binary as a buffer in Elixir kills performance, in the same way using a tuple does. Each time you modify a binary, the VM has to copy it whole, so this is bound to be really slow for big enough inputs. (unless you do it the right way: https://stackoverflow.com/questions/46095870/whats-the-best-way-to-build-long-strings-in-elixir)

As a simple solution, using a list or array and then joining its values once at the end should work.

@Godspower-Eze
Copy link
Contributor Author

Godspower-Eze commented Jan 11, 2024

The failing test is not from this PR. @MegaRedHand can you please take a look?

@MegaRedHand
Copy link
Collaborator

Please pull the newest commits from main. It should have been fixed by #607

Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@MegaRedHand MegaRedHand merged commit c146fec into lambdaclass:main Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SSZ] Support merkleization for lists
2 participants